Skip to content

Reduce integer Display implementation size #133247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Nov 25, 2024
Merged

Reduce integer Display implementation size #133247

merged 2 commits into from
Nov 25, 2024

Conversation

@GuillaumeGomez
Copy link
Member

I was thinking about #128204 and how we could reduce the size of the code and just realized that we didn't need the _fmt method to be implemented on signed integers, which in turns allow to simplify greatly the macro call.

r? @workingjubilee

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 20, 2024
workingjubilee
workingjubilee reviewed Nov 20, 2024
View reviewed changes
@workingjubilee
Copy link
Member

Thanks! r=me with squash.

@GuillaumeGomez GuillaumeGomez force-pushed the reduce-integer-display-impl branch from 75f1ff7 to a991f25 Compare November 20, 2024 22:51
@GuillaumeGomez
Copy link
Member Author

@bors r=workingjubilee

@bors
Copy link
Collaborator

bors commented Nov 20, 2024

📌 Commit a991f25 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 21, 2024
Closed
@matthiaskrgr
Copy link
Member

@bors r-
guess this failed here? #133273 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 21, 2024
@GuillaumeGomez GuillaumeGomez force-pushed the reduce-integer-display-impl branch 2 times, most recently from 81d470f to ab38c6d Compare November 21, 2024 11:35
@GuillaumeGomez
Copy link
Member Author

@bors r=workingjubilee

@bors
Copy link
Collaborator

bors commented Nov 21, 2024

📌 Commit ab38c6d has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 21, 2024
@GuillaumeGomez GuillaumeGomez mentioned this pull request Nov 21, 2024
Closed
@workingjubilee
Copy link
Member

? PR CI doesn't run library tests...?

@workingjubilee
Copy link
Member

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 22, 2024
workingjubilee
workingjubilee reviewed Nov 22, 2024
View reviewed changes
library/core/src/fmt/num.rs
Comment on lines 321 to +310
#[cfg(feature = "optimize_for_size")]
fn $gen_name(mut n: $u, is_nonnegative: bool, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// 2^128 is about 3*10^38, so 39 gives an extra byte of space
let mut buf = [MaybeUninit::<u8>::uninit(); 39];
const SIZE: usize = $u::MAX.ilog(10) as usize + 1;
let mut buf = [MaybeUninit::<u8>::uninit(); SIZE];
Copy link
Member

@workingjubilee workingjubilee Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? What is the effect of this change on binary size? Or is $u always the same?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On smaller targets, the maximum size might be of u32 and not u64 so since we're there... But on the binary size, there should be no difference for this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, hopefully so.

@GuillaumeGomez
Copy link
Member Author

The problem was indeed because I never tested with the optimize_for_size feature. :)

@lukas-code lukas-code mentioned this pull request Nov 23, 2024
Merged
@GuillaumeGomez
Copy link
Member Author

Forgot to re-r+ it...

@bors r=workingjubilee

@bors
Copy link
Collaborator

bors commented Nov 24, 2024

📌 Commit 0d4b52f has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2024
@bors
Copy link
Collaborator

bors commented Nov 25, 2024

⌛ Testing commit 0d4b52f with merge 7db7489...

@bors
Copy link
Collaborator

bors commented Nov 25, 2024

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 7db7489 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 25, 2024
@bors bors merged commit 7db7489 into rust-lang:master Nov 25, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Nov 25, 2024
@GuillaumeGomez GuillaumeGomez deleted the reduce-integer-display-impl branch November 25, 2024 13:48
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7db7489): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.2% [-0.2%, -0.2%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 5.1% [5.1%, 5.1%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.4% [-2.4%, -2.4%] 2
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary -0.0%, secondary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.0% [0.0%, 0.1%] 4
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.0% [-0.1%, -0.0%] 8
Improvements ✅
(secondary)		 -0.1% [-0.1%, -0.0%] 36
All ❌✅ (primary) -0.0% [-0.1%, 0.1%] 12

Bootstrap: 797.447s -> 796.592s (-0.11%)
Artifact size: 336.31 MiB -> 336.27 MiB (-0.01%)

@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 16, 2025
Closed
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 29, 2025
Merged
@Jules-Bertholet Jules-Bertholet mentioned this pull request Sep 9, 2025
Open
@lukaslueg lukaslueg mentioned this pull request Oct 2, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@workingjubilee workingjubilee workingjubilee approved these changes

Assignees

@workingjubilee workingjubilee

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.85.0

Development

Successfully merging this pull request may close these issues.

7 participants

@GuillaumeGomez @workingjubilee @bors @matthiaskrgr @jieyouxu @rust-timer @rustbot